feat: introduce JUnit5 extension #545

lburgazzoli · 2021-09-16T17:00:47Z

No description provided.

lburgazzoli · 2021-09-20T12:38:30Z

@metacosm @csviri what do you think about this extension ?
I think it could be a basis for a testing framework for operator written using the java-operator-sdk

metacosm · 2021-09-20T12:45:43Z

I'm planning on looking at this today.

csviri · 2021-09-20T14:11:10Z

I will take a look tomorrow morning

...-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java

metacosm · 2021-09-21T07:38:00Z

Do you intend to make more changes, @lburgazzoli?

csviri · 2021-09-21T07:40:30Z

...work-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BaseConfigurationService extends AbstractConfigurationService {


I don't get this class. Why are we creating loggers this way?

This class is just used to be able to have a non-abstract implementation that can be used in the junit extension. I'm debating whether to just put the DefaultConfigurationService into core itself and get rid of this class completely but I'm not too sure of the impact on the quarkus extension…

oki,
(will take a look on the quarkus side to understand these aspects better)

csviri · 2021-09-21T07:43:27Z

@metacosm @csviri what do you think about this extension ?
I think it could be a basis for a testing framework for operator written using the java-operator-sdk

Starting to look. Would it be an idea to add description to the PR's such this, where we explain some problem statement, so what is we are solving? What do you think?

csviri · 2021-09-21T07:44:52Z

...-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java

@@ -60,12 +54,14 @@ protected void throwExceptionOnNameCollision(
    final var key = keyFor(controller);
    final var configuration = configurations.get(key);
    if (configuration == null) {
-      log.warn(
-          "Configuration for controller '{}' was not found. {}", key, getControllersNameMessage());
+      logMissingControllerWarning(key, getControllersNameMessage());


Just to my understanding, why is this better?

The benefit is minimal, indeed, just to be able to not pull SLF4J when using AbstractConfigurationService.

Hmm, isn't this a little bit over-engineering? We use SLF4J everywhere, and it's quite an industry standard

The other issue is also that the previous version hardcoded the name of the logger, which might not have been appropriate depending on the actual implementation being executed (for example, even the Quarkus implementation would have logged something with Default ConfigurationService implementation logger name).

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

csviri · 2021-09-21T08:27:34Z

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

+
+  @Override
+  public void beforeAll(ExtensionContext context) throws Exception {
+    before(context);


Why do we execute this before all? Should be this either before each or before all but not both?

the invocation depend on how you instantiate the extension:

if static --> beforeAll

if non static --> beforeEach

(at least that is my understanding)

ahh, ok, thank you!

metacosm · 2021-09-21T08:37:31Z

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

@@ -135,6 +135,7 @@ protected void before(ExtensionContext context) {
    namespace += "-";
    namespace += context.getRequiredTestMethod().getName();
    namespace = KubernetesResourceUtil.sanitizeName(namespace).toLowerCase(Locale.US);
+    namespace = namespace.substring(0, Math.min(namespace.length(), 63));


So the length is not taken into account in sanitizeName? That sounds like a fabric8 bug to me…

Correct, what it does is:

name.replaceAll("[^A-Za-z0-9]+", "-")

Yep, just saw that, will fix it now :)

csviri · 2021-09-21T08:47:11Z

In general I like this idea, looks good, makes the tests more readable. Hopefully won't make it more complex to understand the framework/tests for newcomers.

lburgazzoli mentioned this pull request Sep 16, 2021

feat: create a junit5 extension #507

Closed

lburgazzoli changed the title ~~feat: introduce Junit5 extension~~ feat: introduce JUnit5 extension Sep 16, 2021

lburgazzoli force-pushed the junit-ext-2 branch 2 times, most recently from 7165d2c to 19a95fa Compare September 16, 2021 17:13

feat: introduce Junit5 extension

26a52ea

lburgazzoli force-pushed the junit-ext-2 branch from 19a95fa to 26a52ea Compare September 16, 2021 17:19

lburgazzoli marked this pull request as draft September 20, 2021 12:46

metacosm added 6 commits September 20, 2021 17:17

fix: properly close k8s client when closing the Operator

0380945

fix: catch also CRD application errors, make error message more detailed

a144f01

refactor: widen scope of getCustomResource & rename to getNamedResource

04a12d3

refactor: add create method and use it

39ba19a

fix: improve module name

ab6a4f2

fix: remove unneeded dependencies

fad4baf

lburgazzoli commented Sep 20, 2021

View reviewed changes

...-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java Outdated Show resolved Hide resolved

fix: remove AbstractConfigurationService dependency on slf4j

f46f3d5

metacosm force-pushed the junit-ext-2 branch from 7b6b5a5 to f46f3d5 Compare September 20, 2021 20:26

csviri reviewed Sep 21, 2021

View reviewed changes

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java Outdated Show resolved Hide resolved

fix: use the test class/method to compute the test namespace

d3c5921

lburgazzoli marked this pull request as ready for review September 21, 2021 08:24

csviri reviewed Sep 21, 2021

View reviewed changes

fix: ensure the namespace name never exceed the allowed length

0cc3a32

metacosm reviewed Sep 21, 2021

View reviewed changes

csviri approved these changes Sep 21, 2021

View reviewed changes

metacosm approved these changes Sep 21, 2021

View reviewed changes

metacosm merged commit d518ee3 into operator-framework:master Sep 21, 2021

metacosm deleted the junit-ext-2 branch September 21, 2021 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce JUnit5 extension #545

feat: introduce JUnit5 extension #545

lburgazzoli commented Sep 16, 2021

lburgazzoli commented Sep 20, 2021

metacosm commented Sep 20, 2021

csviri commented Sep 20, 2021

metacosm commented Sep 21, 2021

csviri Sep 21, 2021

metacosm Sep 21, 2021

csviri Sep 21, 2021

csviri commented Sep 21, 2021

csviri Sep 21, 2021

metacosm Sep 21, 2021

csviri Sep 21, 2021

metacosm Sep 21, 2021

csviri Sep 21, 2021

lburgazzoli Sep 21, 2021

csviri Sep 21, 2021

metacosm Sep 21, 2021

lburgazzoli Sep 21, 2021

metacosm Sep 21, 2021

csviri commented Sep 21, 2021

feat: introduce JUnit5 extension #545

feat: introduce JUnit5 extension #545

Conversation

lburgazzoli commented Sep 16, 2021

lburgazzoli commented Sep 20, 2021

metacosm commented Sep 20, 2021

csviri commented Sep 20, 2021

metacosm commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Sep 21, 2021